Skip to content

feat(nimbus): split retention results into fixed weekly metrics - #16512

Merged
RJAK11 merged 3 commits into
mainfrom
16421
Sep 10, 2026
Merged

feat(nimbus): split retention results into fixed weekly metrics#16512
RJAK11 merged 3 commits into
mainfrom
16421

Conversation

@moibra05

@moibra05 moibra05 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Because

  • The existing retention metric only represented Week 2 preventing users from viewing retention for other weeks

This commit

  • Splits retention into fixed weekly metrics, gives each metric the correct displayed window, and hides unrelated breakdowns

Fixes #16421

@moibra05
moibra05 force-pushed the 16421 branch 2 times, most recently from af8e489 to f54659c Compare July 27, 2026 19:47
jaredlockhart
jaredlockhart previously requested changes Jul 27, 2026
View reviewed changes

@jaredlockhart jaredlockhart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay one small labeling change but otherwise I think looks good thanks @moibra05 🙏 🎉

Comment thread experimenter/experimenter/experiments/constants.py Outdated
@moibra05
moibra05 requested a review from jaredlockhart July 28, 2026 21:08
mikewilli
mikewilli requested changes Aug 7, 2026
View reviewed changes

@mikewilli mikewilli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, but looks close. Do we need to do a data migration here as well or is this backwards compatible?

Comment thread experimenter/experimenter/jetstream/client.py Outdated
Comment thread experimenter/experimenter/jetstream/client.py Outdated
Comment thread experimenter/experimenter/jetstream/models.py
Comment thread experimenter/experimenter/jetstream/models.py
Comment thread experimenter/experimenter/jetstream/tests/test_jetstream_data.py
Comment thread experimenter/experimenter/jetstream/tests/test_jetstream_data.py
@RJAK11

RJAK11 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

@mikewilli I don't have the full context so let me know if I misunderstood anything but I just made the changes you mentioned.
Since we rename the key, I think existing results data won’t find the old value anymore. So current experiments would not have the data for Week 2 and completed experiments wouldn’t automatically recover since they don’t re-fetch.
The migration I added fixes that by clearing analysis_start_time once so the normal results task re-fetches from Jetstream and after that the real timestamp gets written back so it only happens once but it seems like a huge task. The other option I could think of would be to rewrite the stored keys directly which avoids the re-fetch, but it edits the existing data in place and seems a bit risky. What do you think?

@RJAK11
RJAK11 requested a review from mikewilli August 26, 2026 15:18
mikewilli
mikewilli approved these changes Aug 31, 2026
View reviewed changes

@mikewilli mikewilli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up! Changes look good, left one small question about a possible tiny refactor.

As for the migration, I think the approach you chose is probably our best option. The results fetches are quick (couple seconds at most), so a one-time re-fetch of everything shouldn't be a big deal. We're also fetching everything automatically anyway, so there's no concern about accidentally overwriting results. I'd be interested in @jaredlockhart 's input in case he has a different opinion, but in lieu of that I'd say go ahead with your chosen approach.

Comment thread experimenter/experimenter/jetstream/client.py
moibra05 and others added 3 commits September 10, 2026 12:08
@moibra05 @RJAK11
Revert "fix(nimbus): fix retention displaying incorrect values (#15351)"
db94746 
This reverts commit 1e49335.
@moibra05 @RJAK11
feat(nimbus): split retention results into fixed weekly metrics
1872126
@RJAK11
feat(nimbus): split retention results into fixed weekly metrics
01ce561 
Because

- The existing retention metric only represented Week 2 preventing users from viewing retention for other weeks

This commit

- Splits retention into fixed weekly metrics, gives each metric the correct displayed window, and hides unrelated breakdowns

Fixes #16421
@RJAK11

RJAK11 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor
Screen.Recording.2026-09-10.at.12.45.37.PM.mov

@RJAK11
RJAK11 dismissed jaredlockhart’s stale review September 10, 2026 19:50

All the requested changes have been made

@RJAK11
RJAK11 added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 5046971 Sep 10, 2026
52 of 53 checks passed
@RJAK11
RJAK11 deleted the 16421 branch September 10, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikewilli mikewilli mikewilli approved these changes
@freshstrangemusic freshstrangemusic Awaiting requested review from freshstrangemusic freshstrangemusic is a code owner
@RJAK11 RJAK11 Awaiting requested review from RJAK11 RJAK11 is a code owner
@yashikakhurana yashikakhurana Awaiting requested review from yashikakhurana yashikakhurana is a code owner
@jaredlockhart jaredlockhart Awaiting requested review from jaredlockhart jaredlockhart is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Split 'retained' into separate fixed windows

4 participants

@moibra05 @RJAK11 @jaredlockhart @mikewilli